Fix repeated string serialization for JSON. #6888
Open
+55
−7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6888 +/- ##
============================================
- Coverage 90.26% 90.23% -0.03%
- Complexity 6586 6594 +8
============================================
Files 729 729
Lines 19767 19800 +33
Branches 1944 1947 +3
============================================
+ Hits 17842 17867 +25
- Misses 1334 1341 +7
- Partials 591 592 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
cc @jhalliday |
jack-berg
force-pushed
the
wip-support-repeated-string
branch
from
b6d8daf
to
b9176bf
Compare
jack-berg
reviewed
Nov 22, 2024
| @@ -304,6 +304,8 @@ private static Profile.Builder sampleProfileBuilder() { | |||
| .AGGREGATION_TEMPORALITY_CUMULATIVE) | |||
| .build()) | |||
| .addAllComment(listOf(8L, 9L)) | |||
| .addStringTable("foo") | |||
| .addStringTable("bar") | |||
There was a problem hiding this comment.
This adds test coverage for the binary encoding. Making this change on main causes tests to fail, but the tests pass on this branch, indicating your fix is successful.
jack-berg
approved these changes
Nov 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes an issue where repeated strings would serialize as the same JSON field multiple times (the same encoding used in binary proto).
This is because in JSON string is a primitive, but not in binary Protocol buffers.
E.g. instead of
stringTable: ["one", "two"],, we'd getstringTable: "one", stringTable: "two".serializeRepeatedStringabstract method which can appropriately handled string arrays in JSON.sizeRepatedStringfor solidarity with other methods.For maintainers - This code relies on existing protobuf tests. If you'd like to see the serialization primitives start to have test cases, I'd have to build up a test suite for them but happy to do so.